Hi @ikq,

JsSIP code must have the same style in order to make it readable and maintainable. Comments before making a deeper review:

• Debug line as the first method call in al public method implementations.
• New line after return.
• New line before and after call to super().
• First conditional in a block starts in new line and last ends in a new line.
• Conditional blocks always contain curly brackets.
• Comments in the line above when possible.
• Calls to debug have usually a new line above and a new line below.

Please take a certain file (ie: RTCSession.js) as a reference and try to stick to its style.

@ikq

Thanks for the cleanup, 👍 We'll review the PR as we can.

@ikq,

Cosmetic comment, we use camelCase in API method arguments (not in 100% of the cases, I know, but we will get there). Could you please make such change?

I mostly fixed the issues.

[I thought about dialog:
if you write in C++/java style, then you could add an abstract class dialog, and invite-dialog, subscriber and notifier dialogs would be his descendants.
The abstract dialog class could be something like stripped version of Dialog.js without actual method implementations]

About dialog, seems:

dialog interface:

• id (string)
• state (number)
• receiveRequest receive incoming request with the id

other functionality:

• check incoming requests
• keep set of parameters to create outgoing request.

It's already implemented in Subscriber/Notifier.
[Not exactly as in Dialog.js, but the same functionality]

Hi,

Super busy lately. Will come back to this when we have some time.

H @ikq,

Dialog should be agnostic to the method. Why can't current Dialog implementation be used here? It has basically the sendRequest() method that you can call, and it will call the receiveRequest() method on your Subscriber/Notifier implementation.

Hmm...
I checked the Dialog.js code again.
sendRequest and receiveRequest can be used.

I just did not consider this possibility before because I found INVITE/CANCEL/ACK specific code and properties in the Dialog (e.g. subscriber used different dialog states set).

There is INVITE related code in Dialog because a dialog for INVITE method needs to behave in certain way, different to generic dialogs, but that's an implementation detail. Other clases should be able to create dialogs the same way RTCSession does.

Hi Jose,
Now subscriber and notifier used Dialog.js
It passed my basic testing now.

I'll continue to test next week:

• I cannot check subscriber authentication (my sip server don't request it).
• subscriber with some production server.
• subscriber with notifier.

Testing of the current version has been completed successfully.
(subscribe authentication was not checked)

subscribe authentication checked using special test.
IMHO: the subscriber/notifier is ready.

I see I should change debug/debugerror to logger...

Hi @ikq,

We are super busy lately. Please keep using your branch and update this PR whenever you find any issue/enhancement.

Also, tests would accelerate the adoption.

I already use this code for my work and so far no problem.

Two browsers test can be taken from:
https://github.com/ikq/subscribe_notify_test

@ikq, more than an external test I mean tests in JsSIP as part of this PR, testing the clases created here.

Hi, is there some news? I'd like to test this functionality.

Hi, is there some news? I'd like to test this functionality.

IMHO it's stable code.
[At least we use subscriber in our production, currently no issues reported]

The subscriber/notifier test can taken from https://github.com/ikq/subscribe_notify_test

Hi, is there a specific reason why Subscriber and Notifier don't use the Contact header from the UA like RTCSession does? If the user provides a custom contact_uri in the UAConfiguration he would have to manually add it as a extra header to Subscriber and Notifier again, wouldn't he?

Hi, is there some news? I'd like to test this functionality.

IMHO it's stable code.
[At least we use subscriber in our production, currently no issues reported]

The subscriber/notifier test can taken from https://github.com/ikq/subscribe_notify_test

I'd like to use the official repository, so I'd like if it will be merged ASAP

Hi, is there a specific reason why Subscriber and Notifier don't use the Contact header from the UA like RTCSession does? If the user provides a custom contact_uri in the UAConfiguration he would have to manually add it as a extra header to Subscriber and Notifier again, wouldn't he?

Yes, I'm not using the Configuration.contact_uri

In fact, I was in a hurry to do this work, and did not pay attention at all to that there is such a parameter in the configuration !

Let check, how the configuration used for INVITE:

UA.js:
   this._contact = {
      pub_gruu  : null,
      temp_gruu : null,
      uri       : this._configuration.contact_uri,

RTCSession.js:
  this._contact = this._ua.contact.toString({
      anonymous,
      outbound : true
    });

Thanks.
It can be improved (use of Configuration.contact_uri)

P.S.
Usage Configuration.contact_uri will modify default SUBSCRIBE contact - will be missed "+sip.instance" (that I add to default SUBSCRIBE Contact)
User still can set "+sip.instance" to SUBSCRIBE Contact via custom Contact header.

@ikq if you don't mind external contributions i'd be willing to take a swing at writing unit tests for this project's test framework

if you don't mind external contributions i'd be willing to take a swing at writing unit tests for this project's test framework

Thank you, I will gladly accept your help.

I have provided an external test for 2 browsers: one is sending an SUBSCRIBE and the other is receiving it and sending a NOTIFY.

Seems in unit test should be simulated 2 instances of JsSIP one send/other receive.
[I don't know how it can be written.
Probably instead two JsSIP instances, can be used single that send the message itself ?]
;-)

Thanks.
It can be improved (use of Configuration.contact_uri)

P.S.
Usage Configuration.contact_uri will modify default SUBSCRIBE contact - will be missed "+sip.instance" (that I add to default SUBSCRIBE Contact)
User still can set "+sip.instance" to SUBSCRIBE Contact via custom Contact header.

@ikq Is there a specific reason to use Configuration.contact_uri? Why not just do it as in RTCSession, with this._ua.contact.toString? Then it would also use the gruu if one was provided. Or am I missing something?

Thank you markusatm for founding the problem.
Thank you stefang42, I use your fix to build Contact in subscriber and notifier.

Hi,

Yes, in order for this PR to be merged we need tests in JsSIP project.

You don't need two JsSIP instances, just one instance and you should inject a transport that you control (a FAKE transport) that you use to send JsSIP UA SUBSCRIBE|NOTIFY messages and receive and check what comes from JsSIP.

If you need some help please let us know.

Hi,
I added nodeunit test to test subscriber notifier communication.

1. Implemented loopSocket that send message itself, but modified Call-ID in each leg.
   (without the modification method findDialog confuses SUBSCRIBE dialog with NOTIFY dialog)

2. subscriber and notifier run in the same JsSIP instance and send/receive SIP messages.

The test SIP sequence is:

• SUBSCRIBE
• SUBSCRIBE OK
• NOTIFY
• NOTIFY OK
• unSUBSCRIBE (with expires 0)
• unSUBSCRIBE OK
• final NOTIFY (with Subscription-State: terminated)
• final NOTIFY OK.

The unit test work silently, but if uncomment line JsSIP.debug.enable('JsSIP:*');
can be seen complete SIP communication.

SUBSCRIBE and NOTIFY headers checking:
url, method, contact, subscribe accept, notify subscription-state, content-type and body.
Checked order of event sequence.

Also please check the test error given by CI.

NOTE: This is taking time @ikq @orgads, but the result is getting very nice. Thanks for your patience and dedication!

Thanks @jmillan and @orgads.

About SIP SUBSCRIBE, it can be used not only to open session and refresh before expiration,
but also can provide some information in SUBSCRIBE body. (in the same way as in NOTIFY).

You can send at random time SUBSCRIBE to provide to other side updated body.

@ikq , @orgads I find the current status OK. Let me please ask you to go though this PR line by line once again and confirm that everything is fine.

Are you using this code on your product on a daily basis?

EDIT: Feel free to write some more tests as it will be a safeguard for the future maintenance of the code.